Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resurrect the docker image build/push to GHCR #1420

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

bickelj
Copy link
Contributor

@bickelj bickelj commented Jan 8, 2025

Prior to this commit, the Digital Ocean deployment pipeline would pull the new source code, re-run lint, re-run tests, build a docker image, and then deploy. The problem with this approach is that it includes too much, does too much, and is entirely outside our CI/CD steps at the canonical source forge (GH). The problem is revealed when running the lint step at Digital Ocean: no Python environment. But then an attempt to add Python to the environment both fails and does not make much sense. There doesn't need to be a Python environment, nor any development environment for that matter, present in the production deployment image. Only the already-linted, already-compiled, and already-tested code should go into production. Instead of trying to remove steps from the DO pipeline, this commit restores a docker image build and push when a merge to main occurs. Note that a controversial token is not used in this step and the inter-repository trigger that previously used such a token not restored.

The approach here is to build an artifact out of components that have already been linted, compiled, and tested, and do not include any development tools along the way. See the npm prune --omit=dev command. Note that this does not perfectly meet the objective but further improvements can be done in later commits, for example, making this step contingent on previous workflow steps and declaring uses the exact same deployment image in the lint and test workflows.

On the DO App Platform side, there is an option to run a docker image rather than compiling and building one from source. Other benefits come from this approach. The version of node used by DO was not clear until looking deep into logs or running a shell on the production runtime. The version of node was not able to controlled, either. With the approach of running an existing docker image, the full runtime, including node version, is known and controlled. A further benefit is when an issue occurs in production that cannot be easily reproduced with source code, the exact image running in production can be run on a local development environment by pulling from the image repository.

With this commit, the ability to use the option of running a GHCR image on the DO App Platform is present because the image is publicly available.

Issue #1410
PhilanthropyDataCommons/deploy#118 PR #1236

@bickelj bickelj requested a review from slifty January 8, 2025 17:07
@bickelj bickelj force-pushed the resurrect-docker-image-build-push-to-ghcr branch 2 times, most recently from 23df594 to 935ae51 Compare January 8, 2025 17:11
Copy link
Member

@slifty slifty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this!

There are some action lint flags to be addressed; also have a comment about sticking with just major-version pinning (I think we had landed on major being good enough?)

The other thing is: could you remove the Aptfile in this PR since the PR obviates the need for that file.

Dockerfile Outdated Show resolved Hide resolved
@bickelj bickelj force-pushed the resurrect-docker-image-build-push-to-ghcr branch from 935ae51 to bdecd4a Compare January 8, 2025 17:20
@bickelj
Copy link
Contributor Author

bickelj commented Jan 8, 2025

Nice catch by actionlint there.

@bickelj bickelj force-pushed the resurrect-docker-image-build-push-to-ghcr branch from bdecd4a to 0257634 Compare January 8, 2025 17:29
Copy link
Member

@slifty slifty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Left a question about patch stuff but it's not blocking / I defer to whether you think it's consistent to shift it to Major only vs patch, but if you think having patch here is more important than the other places that's fine by me.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@bickelj
Copy link
Contributor Author

bickelj commented Jan 8, 2025

@slifty Good catches. I removed the Aptfile, linting is now passing, and I begrudgingly restored fuzzy node runtime versioning.

Prior to this commit, the Digital Ocean deployment pipeline would pull
the new source code, re-run lint, re-run tests, build a docker image,
and then deploy. The problem with this approach is that it includes
too much, does too much, and is entirely outside our CI/CD steps at
the canonical source forge (GH). The problem is revealed when running
the `lint` step at Digital Ocean: no Python environment. But then an
attempt to add Python to the environment both fails and does not make
much sense. There doesn't need to be a Python environment, nor any
development environment for that matter, present in the production
deployment image. Only the already-linted, already-compiled, and
already-tested code should go into production. Instead of trying to
remove steps from the DO pipeline, this commit restores a docker image
build and push when a merge to main occurs. Note that a controversial
token is not used in this step and the inter-repository trigger that
previously used such a token not restored.

The approach here is to build an artifact out of components that have
already been linted, compiled, and tested, and do not include any
development tools along the way. See the `npm prune --omit=dev`
command. Note that this does not perfectly meet the objective but
further improvements can be done in later commits, for example, making
this step contingent on previous workflow steps and declaring `uses`
the exact same deployment image in the lint and test workflows.

On the DO App Platform side, there is an option to run a docker image
rather than compiling and building one from source. Other benefits
come from this approach. The version of node used by DO was not clear
until looking deep into logs or running a shell on the production
runtime. The version of node was not able to controlled, either. With
the approach of running an existing docker image, the full runtime,
including node version, is known and controlled. A further benefit is
when an issue occurs in production that cannot be easily reproduced
with source code, the exact image running in production can be run on
a local development environment by pulling from the image repository.

With this commit, the ability to use the option of running a GHCR
image on the DO App Platform is present because the image is publicly
available.

Issue #1410
PhilanthropyDataCommons/deploy#118
PR #1236
@bickelj bickelj force-pushed the resurrect-docker-image-build-push-to-ghcr branch from 0257634 to 958f409 Compare January 8, 2025 17:42
@bickelj
Copy link
Contributor Author

bickelj commented Jan 8, 2025

Something I'd like to have in future (no need to spend the time here and now on it) is to run the docker image build steps on every pull request but only push the image on the merge to main. It might also be nice to run some simple tests on the image after build. Along similar lines mentioned in the commit, make this build step contingent on passage of lint, test, etc., also use the same docker base image that we use to deploy in all these github actions.

@bickelj bickelj merged commit 59a48a9 into main Jan 8, 2025
9 checks passed
@bickelj bickelj deleted the resurrect-docker-image-build-push-to-ghcr branch January 8, 2025 17:48
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.30%. Comparing base (41dd330) to head (958f409).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1420   +/-   ##
=======================================
  Coverage   87.30%   87.30%           
=======================================
  Files         209      209           
  Lines        2876     2876           
  Branches      408      408           
=======================================
  Hits         2511     2511           
  Misses        336      336           
  Partials       29       29           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants